Support separate node announcement addresses #484

aylagreystone · 2025-03-14T20:28:58Z

Fixes #422

Users running ldk-node in a container and/or behind a NAT will often want to or be forced to bind to a different address than they announce to the network.

This PR introduces a new announcement_addresses configuration option that operates the same way listening_addresses does but is used for constructing the node announcement message. An empty announcement_addresses will operate exactly the same way as today by defaulting to listening_addresses.

tnull

Thanks for looking into this!

Already looks pretty good, one comment though.

tnull · 2025-03-17T08:31:38Z

src/config.rs

You'll also need to account for the new field in may_announce_channel (and corresponding tests) in this file. We use this method to check whether we allow to open or accept announced channels.

I looked at this but I believe the may_announce_channel is still correct as-is. announcement_addresses doesn't change the behavior of when or if a node can announce channels. That is still purely determined by listening_addresses and alias. The announcement_addresses just allows an override for what addresses are actually announced.

Ah, you are right! In fact, could you extend the node_announce_channel test case to assert that may_announce_channel is still false if listening_addresses is None and announcement_addresses is Some?

May also be good to add a check for this and log+abort early in Builder::build_with_store_internal, so that the user can't ever startup with misconfigured Node where announcement_addresses is set but listening_addresses aren't.

Both of those suggestions sound good. For the misconfigured node do we want a new error variant or should I re-use the existing BuildError::InvalidAnnouncementAddresses? I think it's probably fine as long as there's a log_error with explanation alongside it.

Also, I think the check should not only be against unset listening_addresses but actually be if config.announcement_addrresses.is_some() && !may_announce_channel(&config).

Basically if you set announcement addresses but the node will not announce then it's misconfigured.

Both of those suggestions sound good. For the misconfigured node do we want a new error variant or should I re-use the existing BuildError::InvalidAnnouncementAddresses? I think it's probably fine as long as there's a log_error with explanation alongside it.

Yeah, though InvalidListeningAddresses might make more sense, no?

Basically if you set announcement addresses but the node will not announce then it's misconfigured.

Yeah, that makes sense. FWIW, could do the same for NodeAlias, as you'd also only expect it to be set if you want to announce your node.

Well if the check is if config.announcement_addrresses.is_some() && !may_announce_channel(&config) then we don't know if it's missing listening_addresses or alias and so maybe it makes more sense for the error to be BuildError::InvalidAnnouncementAddresses` in the sense they are invalid because you didn't set the other values? Or we could add a new error specifically for some announcement field set (announcement_addresses or alias) when the node is not going to announce.

Will add the same check for NodeAlias.

Well, maybe we should switch may_announce_channel to return a Result<(), AnnounceError>? Then, we know at the callsites why it can't announce and can take action (mostly log the right thing before returning the corresponding BuildError) based on the error type?

Gave this a shot in the latest commit. The error message handling feels a bit verbose now but I tried to dry it up best I could. Check it out and let me know what you think.

G8XSU · 2025-03-15T01:01:52Z

src/builder.rs

@@ -412,6 +417,18 @@ impl NodeBuilder {
 		Ok(self)
 	}

+	/// Sets the IP address and TCP port which [`Node`] will announce to the gossip network that it accepts connections on.


should we add the 'if unset' note here as well?

G8XSU · 2025-03-15T01:14:49Z

src/lib.rs

@@ -457,7 +457,9 @@ impl Node {
 								continue;
 							}

-							let addresses = if let Some(addresses) = bcast_config.listening_addresses.clone() {
+							let addresses = if let Some(addresses) = bcast_config.announcement_addresses.clone() {


nit: use var names such as listening_addresses and announcement_addresses for better readability.

G8XSU · 2025-03-15T01:17:06Z

src/lib.rs

@@ -457,7 +457,9 @@ impl Node {
 								continue;
 							}

-							let addresses = if let Some(addresses) = bcast_config.listening_addresses.clone() {
+							let addresses = if let Some(addresses) = bcast_config.announcement_addresses.clone() {


We probably need to update may_announce_channel and also the code-points where we check for availability of listening addresses for public-channel/announcements.

I don't think we do. I mentioned this above:

I looked at this but I believe the may_announce_channel is still correct as-is. announcement_addresses doesn't change the behavior of when or if a node can announce channels. That is still purely determined by listening_addresses and alias. The announcement_addresses just allows an override for what addresses are actually announced.

G8XSU · 2025-03-15T01:17:21Z

src/lib.rs

-							let addresses = if let Some(addresses) = bcast_config.listening_addresses.clone() {
+							let addresses = if let Some(addresses) = bcast_config.announcement_addresses.clone() {
+								addresses
+							} else if let Some(addresses) = bcast_config.listening_addresses.clone() {
 								addresses
 							} else {
 								debug_assert!(false, "We checked whether the node may announce, so listening addresses should always be set");


this log will need to be updated.

I'm not sure it does because listening_addresses is the only thing that should always be set.

tnull · 2025-03-18T08:39:49Z

src/config.rs

Ah, you are right! In fact, could you extend the node_announce_channel test case to assert that may_announce_channel is still false if listening_addresses is None and announcement_addresses is Some?

May also be good to add a check for this and log+abort early in Builder::build_with_store_internal, so that the user can't ever startup with misconfigured Node where announcement_addresses is set but listening_addresses aren't.

tnull · 2025-03-18T08:40:45Z

src/lib.rs

@@ -802,6 +804,11 @@ impl Node {
 		self.config.listening_addresses.clone()
 	}

+	/// Returns our own announcement addresses.


Should this always return the addresses we announce, i.e., fallback to listening_addresses if announcement_addresses are None?

Good question. I think that probably makes more sense? If they want to see what the actual announcement_addresses are they can check the node config.

aylagreystone · 2025-03-18T14:00:24Z

Pushed new commits that address all review feedback. Only consideration left imo is whether or not to introduce new error variant to represent misconfigured announcement parameters.

tnull

Excuse the delay here.

Only consideration left imo is whether or not to introduce new error variant to represent misconfigured announcement parameters.

Yeah, that could make sense to have a catchall BuildError that represents any of the failure cases.

The changes mostly look good to me, but we should try to make them a bit less verbose/less redundant. So please just implement Display for AnnounceError.

Also, please clean up your git history. Each feature commit should have a short one-line description followed by a body describing what the change does and why it was necessary (see https://cbea.ms/git-commit/ for a rough guideline). I think it would also make sense to squash some of these commits already.

tnull · 2025-03-25T11:30:44Z

src/builder.rs

 use crate::{io, NodeMetrics};
+use crate::{may_announce_channel, Node};


nit: Please import may_announce_channel from config, not the crate root.

tnull · 2025-03-25T11:31:58Z

src/builder.rs

@@ -412,6 +417,20 @@ impl NodeBuilder {
 		Ok(self)
 	}

+	/// Sets the IP address and TCP port which [`Node`] will announce to the gossip network that it accepts connections on.
+	///
+	/// **Note**: If unset, the `listening_addresses` will be used as the list of addresses to announce.


nit: Can we link to listening_addresses? This is not only convenient for users, but also asserts we'll get cargo doc` failures if the link would ever break going forward.

tnull · 2025-03-25T11:32:40Z

src/builder.rs

@@ -766,6 +785,13 @@ impl ArcedNodeBuilder {
 		self.inner.write().unwrap().set_listening_addresses(listening_addresses).map(|_| ())
 	}

+	/// Sets the IP address and TCP port which [`Node`] will announce to the gossip network that it accepts connections on.


Please align the docs on the uniffi variant with the non-uniffi variant.

tnull · 2025-03-25T11:35:38Z

src/builder.rs

+		let error_msg_for = |prefix: &str| -> String {
+			match err {
+				AnnounceError::MissingNodeAlias =>
+					format!("{} set but the node will not announce itself as node alias is not configured. Please set a node alias.", prefix),


I don't think we always need to specify what is set, but only the failure condition. To this end, please just implement Display for AnnounceError rather than introducing this custom closure.

Hm okay, might need help with the error message because to me the failure condition is that some but not all of the fields required to enable node announcement were set.

We don't really know if their intention was to have the node be announced or not and so just telling them to set node alias or that node alias wasn't configured makes it sound like it's required but it's really only required if they intend to have their node announced.

Not sure if I'm making sense but I will go ahead with implementing Display for AnnounceError and we can finalize the actual messages later.

Hm okay, might need help with the error message because to me the failure condition is that some but not all of the fields required to enable node announcement were set.

We don't really know if their intention was to have the node be announced or not and so just telling them to set node alias or that node alias wasn't configured makes it sound like it's required but it's really only required if they intend to have their node announced.

Ah, I see your point. Well we can still implement Display for AnnounceError, while adding (some, concise) context while logging.

Not sure if I'm making sense but I will go ahead with implementing Display for AnnounceError and we can finalize the actual messages later.

SGTM

aylagreystone · 2025-03-27T16:51:02Z

Ok I incorporated the feedback and added a Display implementation for AnnounceError which simplified some of the error logging.

I also went back and cleaned up my commit history.

tnull

Cool, thanks for addressing the feedback so far!

It would be great if we could add a new test case that simply sets up two nodes, opens an announced channel, and checks that the counterparty will indeed see the correct announcement_addresses and node_alias set in their network graph once it's updated. You should be able to basically copy the setup part from simple_bolt12_send_receive test, as there we already open an announced channel and wait until it propagates. Mind taking a stab at that in a separate/third commit?

tnull · 2025-03-28T10:20:41Z

src/config.rs

@@ -117,6 +117,10 @@ pub struct Config {
 	/// **Note**: We will only allow opening and accepting public channels if the `node_alias` and the
 	/// `listening_addresses` are set.
 	pub listening_addresses: Option<Vec<SocketAddress>>,
+	/// The addresses which the node will announce to the gossip network that it accepts connections on.
+	///
+	/// **Note**: If unset, the `listening_addresses` will be used as the list of addresses to announce.


nit: Also link listening_addresses here.

tnull · 2025-03-28T10:25:04Z

src/builder.rs

+	if let Err(err) = may_announce_channel(&config) {
+		if config.announcement_addresses.is_some() {
+			log_error!(logger, "Announcement addresses were set but some required configuration options for node announcement are missing: {}", err);
+			return Err(match err {


nit: Let's use a variable for the returned error and use the matches! macro.

tnull · 2025-03-28T10:26:07Z

src/config.rs

+pub(crate) enum AnnounceError {
+	MissingNodeAlias,
+	MissingListeningAddresses,
+	MissingBoth,


Let's rename this MissingAliasAndAddresses.

aylagreystone · 2025-03-28T13:55:22Z

Cool, thanks for addressing the feedback so far!

It would be great if we could add a new test case that simply sets up two nodes, opens an announced channel, and checks that the counterparty will indeed see the correct announcement_addresses and node_alias set in their network graph once it's updated. You should be able to basically copy the setup part from simple_bolt12_send_receive test, as there we already open an announced channel and wait until it propagates. Mind taking a stab at that in a separate/third commit?

Have fixed the nits you mentioned above and pushed a new commit that adds a new integration test that configures one node with both listening and announcement addresses and the other node with just listening addresses and asserts that they both receive the expected addresses.

tnull · 2025-03-28T14:22:39Z

src/builder.rs

+	///
+	/// **Note**: If unset, the [`listening_addresses`] will be used as the list of addresses to announce.
+	///
+	/// [`listening_addresses`]: NodeBuilder::set_listening_addresses


Let's use Self::set_listening_addresses here as NodeBuilder is private under the uniffi feature, hence this breaking doc builds.

tnull · 2025-03-28T14:25:47Z

tests/integration_tests_rust.rs

+
+	// Assert that the aliases and addresses match the expected values
+	assert_eq!(
+		node_a_info.announcement_info.as_ref().unwrap().alias(),


This is failing CI as under the uniffi feature the API behaves slightly differntly:

error[E0599]: no method named `alias` found for reference `&ldk_node::graph::NodeAnnouncementInfo` in the current scope --> tests/integration_tests_rust.rs:950:51 | 950 | node_a_info.announcement_info.as_ref().unwrap().alias(), | ^^^^^-- help: remove the arguments | | | field, not a method error[E0599]: no method named `addresses` found for reference `&ldk_node::graph::NodeAnnouncementInfo` in the current scope --> tests/integration_tests_rust.rs:954:51 | 954 | node_a_info.announcement_info.as_ref().unwrap().addresses(), | ^^^^^^^^^-- help: remove the arguments | | | field, not a method error[E0599]: no method named `alias` found for reference `&ldk_node::graph::NodeAnnouncementInfo` in the current scope --> tests/integration_tests_rust.rs:959:51 | 959 | node_b_info.announcement_info.as_ref().unwrap().alias(), | ^^^^^-- help: remove the arguments | | | field, not a method error[E0599]: no method named `addresses` found for reference `&ldk_node::graph::NodeAnnouncementInfo` in the current scope --> tests/integration_tests_rust.rs:963:51 | 963 | node_b_info.announcement_info.as_ref().unwrap().addresses(), | ^^^^^^^^^-- help: remove the arguments | | | field, not a method

We probably need a feature gate here that makes the checks use one or the other API depending on whether uniffi is set or not.

Yeah, just saw these uniffi issues. Will set up separate feature-gated asserts to handle it.

Ok, fixed the test and looks like CI is passing.

In certain node configurations a user might want or need to announce different addresses to the network than the ones they bind their tcp listeners to.

This error type can provide more context to callers about why the node configuration does not support channel announcements.

A new integration test that configures nodes using the new announcement addresses configuration. Ensures that both nodes receive the correct alias and address for nodes using and not using this new feature.

tnull

Thank you very much! LGTM!

tnull reviewed Mar 17, 2025

View reviewed changes

G8XSU reviewed Mar 17, 2025

View reviewed changes

tnull reviewed Mar 18, 2025

View reviewed changes

tnull reviewed Mar 25, 2025

View reviewed changes

aylagreystone force-pushed the announced-addresses branch 2 times, most recently from 36b6ca4 to c98a403 Compare March 27, 2025 16:49

tnull reviewed Mar 28, 2025

View reviewed changes

aylagreystone force-pushed the announced-addresses branch from c98a403 to 68d5bad Compare March 28, 2025 13:53

tnull reviewed Mar 28, 2025

View reviewed changes

aylagreystone added 2 commits March 28, 2025 10:39

Add configuration for node announcement addresses

fb9243a

In certain node configurations a user might want or need to announce different addresses to the network than the ones they bind their tcp listeners to.

Refactor may_announce_channel to return AnnounceError

e260744

This error type can provide more context to callers about why the node configuration does not support channel announcements.

aylagreystone force-pushed the announced-addresses branch from 68d5bad to eea1fa9 Compare March 28, 2025 14:39

Add test for announcement address propagation

5432ed8

A new integration test that configures nodes using the new announcement addresses configuration. Ensures that both nodes receive the correct alias and address for nodes using and not using this new feature.

aylagreystone force-pushed the announced-addresses branch from eea1fa9 to 5432ed8 Compare March 28, 2025 15:13

tnull approved these changes Mar 28, 2025

View reviewed changes

tnull merged commit 7528e91 into lightningdevkit:main Mar 28, 2025
14 checks passed

		use crate::{io, NodeMetrics};
		use crate::{may_announce_channel, Node};

Support separate node announcement addresses #484

Support separate node announcement addresses #484

Conversation

aylagreystone commented Mar 14, 2025

tnull left a comment

Choose a reason for hiding this comment

tnull Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylagreystone Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylagreystone commented Mar 18, 2025

tnull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylagreystone commented Mar 27, 2025

tnull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylagreystone commented Mar 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

tnull Mar 17, 2025 •

edited

Loading

aylagreystone Mar 18, 2025 •

edited

Loading

tnull left a comment •

edited

Loading